Split multi-tenancy quickstart into two projects #1493
Conversation
It refers to the Quarkus guide. The documentation. So you'd need to:
|
There was a problem hiding this comment.
Thanks, that's a nice cleanup. I added a few comments below.
Before merging, I think we'll need:
- Comments to be addressed/dismissed.
- A PR to be sent to the main Quarkus repo to make sure the documentation (
hibernate-orm.adoc) is updated consistently. - Ideally, all commits in this PR to be squashed into at most two: the splitting, and the addition of
@PersistenceUnitExtensionon tenant resolvers. EDIT: Okay maybe three if you want to keep the devservice thing separate, but that seems like it could be annoying to do properly, so I'd just squash that together in the same commit as the splitting.
I created a follow-up issue (quarkusio/quarkus#45715), but it's out of scope: just a few pre-existing problems that your PR reminded me of (thanks!).
There was a problem hiding this comment.
You should probably align this file on the docs, since that's where we'll explain the quickstart: https://quarkus.io/guides/hibernate-orm#database-approach
One thing you'll want to change compared to the docs though (and maybe update in the docs): the JDBC URL property needs to be prefixed with %prod, otherwise dev services will be disabled and Quarkus will just try to connect to that URL.
So this:
quarkus.datasource.base.jdbc.url=jdbc:postgresql://localhost:5432/quarkus_test
...
quarkus.datasource.mycompany.jdbc.url=jdbc:postgresql://localhost:5433/mycompany
Should be this instead:
%prod.quarkus.datasource.base.jdbc.url=jdbc:postgresql://localhost:5432/quarkus_test
...
%prod.quarkus.datasource.mycompany.jdbc.url=jdbc:postgresql://localhost:5433/mycompany
There was a problem hiding this comment.
Same here, ideally the whole file should be made consistent with the one at https://quarkus.io/guides/hibernate-orm#schema-approach
Here too, the JDBC URL should be limited to the prod profile, otherwise dev services will be disabled.
| /** | ||
| * Workaround to allow launching the Quarkus application inside the IDE. | ||
| * | ||
| * @see https://github.com/quarkusio/quarkus/issues/2143 | ||
| */ | ||
| @Disabled("You can enable this pseudo test to run the app in your IDE") | ||
| @QuarkusTest | ||
| public class IdeTestApp { |
There was a problem hiding this comment.
This issue appears to have been fixed. I'd suggest removing this class from both Maven modules?
There was a problem hiding this comment.
You should probably use @PersistenceUnitExtension here too?
|
Also, I tested locally, it works great! Much better experience than the previous docker-compose setup with Maven profiles. |
This comment has been minimized.
This comment has been minimized.
|
Thanks for the detailed review - I will implement all of it soon! |
|
@yrodiere I noticed an additional warning in the database quickstart:
I tried to fix it by adding: When researching I came across: quarkusio/quarkus#23563 (comment). |
tobHai
force-pushed
the
cleanup-hibernate-multi-tenancy-examples
branch
2 times, most recently
from
5c1acb5 to
f680d7c
Compare
tobHai
force-pushed
the
cleanup-hibernate-multi-tenancy-examples
branch
from
f680d7c to
e435a3f
Compare
Fixes quarkusio/quarkus#41759
Check list:
Your pull request:
developmentbranch999-SNAPSHOTversion of Quarkusmvn clean test)mvn clean package -Pnative)mvn clean verify -Pnative)README.mdfile (with build and run instructions)pom.xmlandREADME.mdI split the existing quickstart into two:
Additionally, I used dev services for the database connections.
The only real code change I did was fixing a warning in:
d939238(#1493)I am not sure to which guide the checklist is referring to - do I need to update another project as well?
Edit: updated the docs as well: https://github.com/quarkusio/quarkus/pull/46030/files